Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Efficiently canonicalize InMemoryCache result objects. #7439

Merged
merged 12 commits into from
Dec 16, 2020

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Dec 9, 2020

As explained in #4141 (comment), this change guarantees that any two result objects returned by InMemoryCache will be referentially equal (===) if they are deeply equal (note: the reverse is trivially true), which is an extremely valuable property for UI components that can skip re-rendering when the current input data are identical to the previous data.

In particular, this change means there will be no difference between optimistic mutation results and non-optimistic final results, as long as the optimistic guess was accurate, fixing #4141 and a number of other similar issues.

@@ -337,6 +334,8 @@ export class StoreReader {
return finalResult;
}

private canon = new Canon;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the Canon belongs to the StoreReader, which belongs to the InMemoryCache, unused memory retained by reader.canon can be garbage collected if the cache is discarded.

That said, I'm also contemplating an API to jettison this kind of data without discarding the whole cache, since it can always be recomputed (though the === guarantee will be lost temporarily).

@benjamn benjamn force-pushed the canonical-cache-results branch from f23ffb8 to 7ff1b31 Compare December 15, 2020 18:56
@benjamn benjamn force-pushed the canonical-cache-results branch from f0e3fd8 to 5266120 Compare December 16, 2020 00:19
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is AWESOME @benjamn! Just a few comments below (nothing blocking!). Also, we might want to consider adding a few direct ObjectCanon tests, to verify pass and admit. I know we're testing them indirectly, but a few direct explicit tests could help future maintainers ramp up on ObjectCanon functionality more quickly. Thanks very much!

src/cache/inmemory/object-canon.ts Show resolved Hide resolved
src/cache/inmemory/object-canon.ts Outdated Show resolved Hide resolved
src/cache/inmemory/object-canon.ts Outdated Show resolved Hide resolved
@benjamn benjamn merged commit 779794f into release-3.4 Dec 16, 2020
@benjamn benjamn deleted the canonical-cache-results branch December 16, 2020 19:18
expect(checkLastResult(bResults, bOyez)).toBe(bOyez);
expect(checkLastResult(abResults, a456bOyez)).not.toBe(a456bOyez);
expect(checkLastResult(abResults, a456bOyez)).toBe(a456bOyez);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very Shakespearean of you

benjamn added a commit that referenced this pull request Jan 20, 2021
benjamn added a commit that referenced this pull request Mar 23, 2021
The goal of broadcastWatches is to notify cache watchers of any new data
resulting from cache writes.

However, it's possible for cache writes to invalidate watched queries in a
way that does not result in any differences in the resulting data, so this
watch.lastDiff caching saves us from triggering a redundant broadcast of
exactly the same data again.

Note: thanks to #7439, when two result objects are deeply equal to each
another, they will automatically also be === to each other, which is what
allows us to get away with the !== check in this code.
benjamn added a commit that referenced this pull request Apr 16, 2021
Revisiting PR #6891, which was reverted by commit c2ef68f.

With the introduction of canonical cache results (#7439), the strict
equality check in setDiff is about to become mostly synonymous with deep
equality checking (but much faster to check), so we need to deal with the
consequences of #6891 one way or another.

As evidence that this change now (after #7439) has no observable impact,
notice that we were able to switch back to using !equal(a, b) without
needing to fix/update any failing tests, compared to the few tests that
needed updating in #6891.

However, by switching to deep equality checking, we allow ourselves the
option to experiment with disabling (or periodically clearing) the
ObjectCanon, which would presumably lead to broadcasting more !== but
deeply equal results to cache.watch watchers.

With deep equality checking in setDiff, those !== results will get past
the filter in the same cases canonical objects would, so there's less of a
discrepancy in client behavior with/without canonization enabled.
benjamn added a commit that referenced this pull request May 10, 2021
Although cache result canonization (#7439) is algorithmically efficient
(linear time for tree-shaped results), it does have a computational cost
for large result trees, so you might want to disable canonization for
exceptionally big queries, if you decide the future performance benefits
of result canonization are not worth the initial cost.

Fortunately, this implementation allows non-canonical results to be
exchaged later for canonical results without recomputing the underlying
results, but merely by canonizing the previous results.

Of course, this reuse happens only when the cache has not been modified
in the meantime (the usual result caching/invalidation story, nothing
new), in which case the StoreReader does its best to reuse as many
subtrees as it can, if it can't reuse the entire result tree.
benjamn added a commit that referenced this pull request May 10, 2021
Although cache result canonization (#7439) is algorithmically efficient
(linear time for tree-shaped results), it does have a computational cost
for large result trees, so you might want to disable canonization for
exceptionally big queries, if you decide the future performance benefits
of result canonization are not worth the initial cost.

Fortunately, this implementation allows non-canonical results to be
exchaged later for canonical results without recomputing the underlying
results, but merely by canonizing the previous results.

Of course, this reuse happens only when the cache has not been modified
in the meantime (the usual result caching/invalidation story, nothing
new), in which case the StoreReader does its best to reuse as many
subtrees as it can, if it can't reuse the entire result tree.
benjamn added a commit that referenced this pull request May 12, 2021
Revisiting PR #6891, which was reverted by commit c2ef68f.

With the introduction of canonical cache results (#7439), the strict
equality check in setDiff is about to become mostly synonymous with deep
equality checking (but much faster to check), so we need to deal with the
consequences of #6891 one way or another.

As evidence that this change now (after #7439) has no observable impact,
notice that we were able to switch back to using !equal(a, b) without
needing to fix/update any failing tests, compared to the few tests that
needed updating in #6891.

However, by switching to deep equality checking, we allow ourselves the
option to experiment with disabling (or periodically clearing) the
ObjectCanon, which would presumably lead to broadcasting more !== but
deeply equal results to cache.watch watchers.

With deep equality checking in setDiff, those !== results will get past
the filter in the same cases canonical objects would, so there's less of a
discrepancy in client behavior with/without canonization enabled.
benjamn added a commit that referenced this pull request May 14, 2021
This reimplementation of the stable `stringify` function used by
`getStoreKeyName` builds on the `ObjectCanon` introduced by my PR #7439,
which guarantees canonical objects keys are always in sorted order.
@hwillson hwillson removed this from the MM-2021-06 milestone Jul 29, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants